OCPBUGS-88737: Update PSM/package server to support TLS profiles#1328
Open
tmshort wants to merge 1 commit into
Open
OCPBUGS-88737: Update PSM/package server to support TLS profiles#1328tmshort wants to merge 1 commit into
tmshort wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe package server controller now reads the cluster APIServer TLS profile, converts it into CSV run flags, and threads those flags through CSV reconciliation. It also watches APIServer updates, and the test invocation was updated for the new ChangesPackage server TLS flags
Sequence Diagram(s)sequenceDiagram
participant PackageServerController
participant configv1.APIServer
participant reconcileCSV
participant ensureCSV
participant manifests.NewPackageServerCSV
PackageServerController->>configv1.APIServer: Get "cluster"
configv1.APIServer-->>PackageServerController: TLSSecurityProfile
PackageServerController->>reconcileCSV: pass TLS run flags
reconcileCSV->>ensureCSV: forward flags
ensureCSV->>manifests.NewPackageServerCSV: build CSV with runFlags
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tmshort The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/package-server-manager/controller.go (1)
87-116:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep
--intervalout of the extra flags passed toensureCSV.
flagsalready includes--intervalhere, andensureCSVprependsintervalagain before appending itsflagsargument. Whenr.Intervalis set, the reconciled CSV command ends up with duplicate--intervalargs. Keep the controller’s TLS flags separate from the full run flag list.Proposed fix
- flags := []string{} - if r.Interval != "" { - flags = append(flags, "--interval", r.Interval) - } + extraFlags := []string{} var apiServer configv1.APIServer if err := r.Client.Get(ctx, types.NamespacedName{Name: "cluster"}, &apiServer); err != nil { if !apierrors.IsNotFound(err) { return ctrl.Result{}, err } } else { minVersion, cipherSuites := olmapiserver.GetSecurityProfileConfig(apiServer.Spec.TLSSecurityProfile) - flags = append(flags, + extraFlags = append(extraFlags, "--tls-min-version", libcrypto.TLSVersionToNameOrDie(minVersion), "--tls-cipher-suites", strings.Join(libcrypto.CipherSuitesToNamesOrDie(cipherSuites), ","), ) } + runFlags := []string{} + if r.Interval != "" { + runFlags = append(runFlags, "--interval", r.Interval) + } + runFlags = append(runFlags, extraFlags...) + required, err := manifests.NewPackageServerCSV( manifests.WithName(r.Name), manifests.WithNamespace(r.Namespace), manifests.WithImage(r.Image), - manifests.WithRunFlags(flags), + manifests.WithRunFlags(runFlags), ) if err != nil { log.Error(err, "failed to serialize a new packageserver csv from the base YAML manifest") return ctrl.Result{}, err } res, err := controllerutil.CreateOrUpdate(ctx, r.Client, required, func() error { - return reconcileCSV(r.Log, r.Image, r.Interval, flags, required, highAvailabilityMode) + return reconcileCSV(r.Log, r.Image, r.Interval, extraFlags, required, highAvailabilityMode) })-func reconcileCSV(log logr.Logger, image string, interval string, flags []string, csv *olmv1alpha1.ClusterServiceVersion, highAvailabilityMode bool) error { +func reconcileCSV(log logr.Logger, image string, interval string, extraFlags []string, csv *olmv1alpha1.ClusterServiceVersion, highAvailabilityMode bool) error { if csv.ObjectMeta.CreationTimestamp.IsZero() { log.Info("attempting to create the packageserver csv") } - modified, err := ensureCSV(log, image, interval, flags, csv, highAvailabilityMode) + modified, err := ensureCSV(log, image, interval, extraFlags, csv, highAvailabilityMode)Also applies to: 143-148
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/package-server-manager/controller.go` around lines 87 - 116, The flags variable contains both the --interval argument and TLS security flags that are passed to reconcileCSV, but reconcileCSV already prepends --interval before appending its flags argument, causing duplicate --interval arguments when r.Interval is set. Refactor the code to separate concerns by creating a separate variable (e.g., tlsFlags) to hold only the TLS configuration flags from the APIServer spec, then pass only the tlsFlags to reconcileCSV instead of the combined flags list. The --interval flag should be handled by reconcileCSV independently. Apply this same refactoring to both locations where this pattern occurs (the main location at lines 87-116 and the consolidated site at lines 143-148).
🧹 Nitpick comments (1)
pkg/package-server-manager/controller_test.go (1)
264-264: ⚡ Quick winAdd a non-nil flags case for the new run-flag path.
This call only exercises the legacy
nilpath, so the new TLS flag merge behavior is untested. Add a table case with extra flags, e.g.--tls-min-versionand--tls-cipher-suites, and assert the expected CSV command contains--intervalfollowed by those flags exactly once.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/package-server-manager/controller_test.go` at line 264, The current test case for ensureCSV only tests the legacy path with nil flags, leaving the new TLS flag merge behavior untested. Add a new table-driven test case to the existing test that provides non-nil flags containing TLS options (such as --tls-min-version and --tls-cipher-suites) to the ensureCSV function, and assert that the resulting CSV command includes the --interval flag followed by those TLS flags appearing exactly once in the output. This ensures the flag merging behavior works correctly for the new run-flag path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@staging/operator-lifecycle-manager/pkg/package-server/server/server.go`:
- Around line 350-369: The applyClusterTLSProfile function makes API discovery
and APIServers().Get calls using the long-lived Run context without timeouts,
which could hang startup indefinitely. Create a short timeout context and a
copied rest.Config with a Timeout field set, then use these for the
IsAPIAvailable and APIServers().Get calls instead of the original context and
config. This ensures that stalled API calls cannot block PackageServer startup.
- Line 9: Remove the redundant join/split operation that wraps the result of
CipherSuitesToNamesOrDie. When CipherSuitesToNamesOrDie returns an empty slice,
the current join/split converts it to a slice with an empty string element,
which causes downstream parsing to fail. Find where CipherSuitesToNamesOrDie is
being called and joined/split, and instead assign its result directly to the
cipher suites variable without the join/split conversion. Then remove the
now-unused strings import from the imports section.
- Around line 225-231: The error handling in the applyClusterTLSProfile call
currently logs a warning and allows startup with default TLS settings when the
cluster profile cannot be applied, which undermines cluster security policies.
Change this to fail closed by updating the error handling logic: instead of
logging a warning and continuing, return the error or log it at a fatal level to
prevent startup with insecure defaults. Additionally, add the required RBAC rule
to the PackageServer ClusterServiceVersion to grant the cluster service account
permission to read apiservers resources in config.openshift.io, which will allow
applyClusterTLSProfile to succeed and retrieve the cluster's TLS configuration.
---
Outside diff comments:
In `@pkg/package-server-manager/controller.go`:
- Around line 87-116: The flags variable contains both the --interval argument
and TLS security flags that are passed to reconcileCSV, but reconcileCSV already
prepends --interval before appending its flags argument, causing duplicate
--interval arguments when r.Interval is set. Refactor the code to separate
concerns by creating a separate variable (e.g., tlsFlags) to hold only the TLS
configuration flags from the APIServer spec, then pass only the tlsFlags to
reconcileCSV instead of the combined flags list. The --interval flag should be
handled by reconcileCSV independently. Apply this same refactoring to both
locations where this pattern occurs (the main location at lines 87-116 and the
consolidated site at lines 143-148).
---
Nitpick comments:
In `@pkg/package-server-manager/controller_test.go`:
- Line 264: The current test case for ensureCSV only tests the legacy path with
nil flags, leaving the new TLS flag merge behavior untested. Add a new
table-driven test case to the existing test that provides non-nil flags
containing TLS options (such as --tls-min-version and --tls-cipher-suites) to
the ensureCSV function, and assert that the resulting CSV command includes the
--interval flag followed by those TLS flags appearing exactly once in the
output. This ensures the flag merging behavior works correctly for the new
run-flag path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f464048d-8014-4442-bdac-70924ccf18bb
📒 Files selected for processing (4)
pkg/package-server-manager/config.gopkg/package-server-manager/controller.gopkg/package-server-manager/controller_test.gostaging/operator-lifecycle-manager/pkg/package-server/server/server.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/package-server-manager/controller.go (1)
87-90:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep
flagsas TLS-only before passing it toensureCSV.
ensureCSVnow receivesintervalseparately and combines it with caller-provided flags. Because this slice already contains--interval, the reconciled CSV can get duplicate interval arguments whilemanifests.WithRunFlagsappends them verbatim.Proposed fix
- flags := []string{} + flags := []string{} + runFlags := []string{} if r.Interval != "" { - flags = append(flags, "--interval", r.Interval) + runFlags = append(runFlags, "--interval", r.Interval) } @@ flags = append(flags, "--tls-min-version", minVersionStr, "--tls-cipher-suites", cipherSuitesStr, ) log.Info("applying cluster TLS security profile to packageserver", "minVersion", minVersionStr, "cipherSuites", cipherSuitesStr) } + runFlags = append(runFlags, flags...) required, err := manifests.NewPackageServerCSV( manifests.WithName(r.Name), manifests.WithNamespace(r.Namespace), manifests.WithImage(r.Image), - manifests.WithRunFlags(flags), + manifests.WithRunFlags(runFlags), ) @@ - return reconcileCSV(r.Log, r.Image, r.Interval, flags, required, highAvailabilityMode) + return reconcileCSV(r.Log, r.Image, r.Interval, flags, required, highAvailabilityMode)Also applies to: 101-104, 108-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/package-server-manager/controller.go` around lines 87 - 90, The flags slice is currently being populated with both TLS and interval arguments before being passed to ensureCSV, but ensureCSV now receives interval separately and combines it with the caller-provided flags, causing duplicate interval arguments. Remove the code blocks that append interval flags to the flags slice. In the section around lines 87-90 in pkg/package-server-manager/controller.go where the code checks if r.Interval is not empty and appends "--interval" and r.Interval to flags, delete this entire conditional block. Apply the same fix at lines 101-104 and lines 108-119 to remove any similar interval flag appending. Keep only TLS-related flags in the flags slice before passing it to ensureCSV, since the interval will be passed separately to that function.
🧹 Nitpick comments (1)
pkg/package-server-manager/controller_test.go (1)
264-264: ⚡ Quick winAdd coverage for non-nil CSV run flags.
This updates the call site but still only tests the old
nilpath. Please add at least one case with TLS flags soensureCSVproves it preserves--tls-min-version/--tls-cipher-suitesin the PackageServer command.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/package-server-manager/controller_test.go` at line 264, The test at the ensureCSV function call is still only testing the nil path for CSV run flags. Add at least one test case to the test suite where non-nil CSV run flags containing TLS parameters (such as --tls-min-version and --tls-cipher-suites) are passed to the ensureCSV function, and verify that these TLS flags are properly preserved in the resulting PackageServer command output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/package-server-manager/controller.go`:
- Around line 87-90: The flags slice is currently being populated with both TLS
and interval arguments before being passed to ensureCSV, but ensureCSV now
receives interval separately and combines it with the caller-provided flags,
causing duplicate interval arguments. Remove the code blocks that append
interval flags to the flags slice. In the section around lines 87-90 in
pkg/package-server-manager/controller.go where the code checks if r.Interval is
not empty and appends "--interval" and r.Interval to flags, delete this entire
conditional block. Apply the same fix at lines 101-104 and lines 108-119 to
remove any similar interval flag appending. Keep only TLS-related flags in the
flags slice before passing it to ensureCSV, since the interval will be passed
separately to that function.
---
Nitpick comments:
In `@pkg/package-server-manager/controller_test.go`:
- Line 264: The test at the ensureCSV function call is still only testing the
nil path for CSV run flags. Add at least one test case to the test suite where
non-nil CSV run flags containing TLS parameters (such as --tls-min-version and
--tls-cipher-suites) are passed to the ensureCSV function, and verify that these
TLS flags are properly preserved in the resulting PackageServer command output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2d4ac933-857f-4c78-a0a5-bac94d1a2b9e
📒 Files selected for processing (4)
pkg/package-server-manager/config.gopkg/package-server-manager/controller.gopkg/package-server-manager/controller_test.gostaging/operator-lifecycle-manager/pkg/package-server/server/server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/package-server-manager/config.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/package-server-manager/controller_test.go (1)
264-264: ⚡ Quick winAdd a non-nil flags case for the new
ensureCSVcontract.Passing only
nilpreserves the old behavior but does not cover the TLS flag path this PR adds. Add a case that passes--tls-min-version/--tls-cipher-suitesand asserts the PackageServer command contains them exactly once, in the expected order.Suggested test coverage direction
for _, tc := range tt { tc := tc t.Run(tc.name, func(t *testing.T) { - gotBool, gotErr := ensureCSV(logger, image, interval, nil, tc.inputCSV, tc.highlyAvailable) + gotBool, gotErr := ensureCSV(logger, image, interval, nil, tc.inputCSV, tc.highlyAvailable) require.EqualValues(t, tc.want.expectedBool, gotBool) require.EqualValues(t, tc.want.expectedErr, gotErr) require.EqualValues(t, tc.inputCSV.Spec, tc.expectedCSV.Spec) }) } + + t.Run("IncludesAdditionalRunFlags", func(t *testing.T) { + flags := []string{ + "--tls-min-version", "VersionTLS12", + "--tls-cipher-suites", "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", + } + inputCSV := newTestCSV() + + gotBool, gotErr := ensureCSV(logger, image, interval, flags, inputCSV, true) + require.NoError(t, gotErr) + require.True(t, gotBool) + + command := inputCSV.Spec.InstallStrategy.StrategySpec.DeploymentSpecs[0].Spec.Template.Spec.Containers[0].Command + require.Equal(t, 1, countOccurrences(command, "--tls-min-version")) + require.Equal(t, 1, countOccurrences(command, "--tls-cipher-suites")) + require.Contains(t, command, "VersionTLS12") + require.Contains(t, command, "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256") + }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/package-server-manager/controller_test.go` at line 264, The current test at line 264 only passes nil flags to the ensureCSV function, which does not exercise the TLS flag path newly added in this PR. Add a test case that calls ensureCSV with non-nil flags containing --tls-min-version and --tls-cipher-suites flags, and then assert that the resulting PackageServer command includes these flags exactly once each in the correct order. This ensures the new TLS flag handling logic is properly covered by tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/manifests/csv.yaml`:
- Line 8: The olm.version field is hardcoded with a commit-derived prerelease
version that differs from the standard versioning pattern used throughout the
codebase (such as 0.0.1-snapshot or release versions like 0.18.3). Regenerate
the CSV version using the proper release versioning flow and standard version
pattern instead of the hardcoded commit hash to ensure correct PackageServer CSV
upgrade ordering. This change applies to all instances of the hardcoded version
string in the manifest file.
- Around line 72-77: The APIServer RBAC rules across three files currently only
grant the `get` verb, but the controller registers a watch on configv1.APIServer
via SharedInformer which requires `list` and `watch` permissions. Update the
verbs array in all three locations to include these missing permissions. In
pkg/manifests/csv.yaml at lines 72-77, add `list` and `watch` verbs to the
apiservers rule. Apply the same change to
staging/operator-lifecycle-manager/deploy/chart/templates/_packageserver.clusterserviceversion.yaml
at lines 70-75, and to
staging/operator-lifecycle-manager/deploy/upstream/quickstart/olm.yaml at lines
267-272. Each location has the same RBAC block with apiGroups containing
"config.openshift.io" and resources containing "apiservers" that needs to be
updated.
---
Nitpick comments:
In `@pkg/package-server-manager/controller_test.go`:
- Line 264: The current test at line 264 only passes nil flags to the ensureCSV
function, which does not exercise the TLS flag path newly added in this PR. Add
a test case that calls ensureCSV with non-nil flags containing --tls-min-version
and --tls-cipher-suites flags, and then assert that the resulting PackageServer
command includes these flags exactly once each in the correct order. This
ensures the new TLS flag handling logic is properly covered by tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 118a19da-b64d-475e-9b9c-3f9a5bbeebaf
📒 Files selected for processing (7)
pkg/manifests/csv.yamlpkg/package-server-manager/config.gopkg/package-server-manager/controller.gopkg/package-server-manager/controller_test.gostaging/operator-lifecycle-manager/deploy/chart/templates/_packageserver.clusterserviceversion.yamlstaging/operator-lifecycle-manager/deploy/upstream/quickstart/olm.yamlstaging/operator-lifecycle-manager/pkg/package-server/server/server.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/package-server-manager/config.go
- staging/operator-lifecycle-manager/pkg/package-server/server/server.go
- pkg/package-server-manager/controller.go
| namespace: openshift-operator-lifecycle-manager | ||
| labels: | ||
| olm.version: 0.0.1-snapshot | ||
| olm.version: 0.0.0-69a5c8cdf920607f2c125bdf0d9e644b8f980dc6 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find committed PackageServer CSV version stamps and check whether this
# commit-derived version is isolated to generated artifacts or part of the intended release flow.
rg -n --iglob '*.yaml' --iglob '*.yml' \
'olm\.version:|^[[:space:]]*version: 0\.0\.0-[0-9a-f]{40}|0\.0\.1-snapshot|0\.0\.0-69a5c8cdf920607f2c125bdf0d9e644b8f980dc6'Repository: openshift/operator-framework-olm
Length of output: 2849
Regenerate the CSV version using the intended release versioning flow.
Lines 8 and 171 hardcode a commit-derived prerelease version (0.0.0-69a5c8cdf920607f2c125bdf0d9e644b8f980dc6) that differs from the standard pattern used elsewhere (0.0.1-snapshot in manifests, {{ .Chart.Version }} in Helm templates, 0.18.3 in upstream releases). This isolated version stamp suggests it came from a local test build and will unintentionally affect PackageServer CSV upgrade ordering.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/manifests/csv.yaml` at line 8, The olm.version field is hardcoded with a
commit-derived prerelease version that differs from the standard versioning
pattern used throughout the codebase (such as 0.0.1-snapshot or release versions
like 0.18.3). Regenerate the CSV version using the proper release versioning
flow and standard version pattern instead of the hardcoded commit hash to ensure
correct PackageServer CSV upgrade ordering. This change applies to all instances
of the hardcoded version string in the manifest file.
Comment on lines
+72
to
+77
| - apiGroups: | ||
| - "config.openshift.io" | ||
| resources: | ||
| - apiservers | ||
| verbs: | ||
| - get |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm the ServiceAccount that runs the APIServer watcher has
# list/watch on config.openshift.io/apiservers, not only get.
echo "Controller APIServer watch/read sites:"
rg -n -C5 --type=go 'configv1\.APIServer|Watches\(.*APIServer|For\(.*APIServer'
echo
echo "RBAC rules mentioning config.openshift.io/apiservers:"
rg -n -C6 --iglob '*.yaml' --iglob '*.yml' 'config\.openshift\.io|apiservers|olm-operator-serviceaccount'Repository: openshift/operator-framework-olm
Length of output: 50389
🏁 Script executed:
echo "=== pkg/manifests/csv.yaml (lines 60-85) ==="
sed -n '60,85p' pkg/manifests/csv.yaml
echo -e "\n=== staging/operator-lifecycle-manager/deploy/chart/templates/_packageserver.clusterserviceversion.yaml (lines 58-85) ==="
sed -n '58,85p' staging/operator-lifecycle-manager/deploy/chart/templates/_packageserver.clusterserviceversion.yaml
echo -e "\n=== staging/operator-lifecycle-manager/deploy/upstream/quickstart/olm.yaml (lines 255-280) ==="
sed -n '255,280p' staging/operator-lifecycle-manager/deploy/upstream/quickstart/olm.yaml
echo -e "\n=== Check which ServiceAccount runs the manager in controller.go ==="
rg -n 'ServiceAccountName|ServiceAccount' pkg/package-server-manager/controller.go -A 2 -B 2 | head -30Repository: openshift/operator-framework-olm
Length of output: 2768
Add list and watch verbs to the APIServer RBAC rules. The added rules grant only get, which works for direct reads, but the controller registers a watch on configv1.APIServer (controller.go:201) via a SharedInformer. Informers require list and watch permissions to function.
Update all three RBAC definitions to include these verbs:
pkg/manifests/csv.yaml#L72-L77staging/operator-lifecycle-manager/deploy/chart/templates/_packageserver.clusterserviceversion.yaml#L70-L75staging/operator-lifecycle-manager/deploy/upstream/quickstart/olm.yaml#L267-L272
Example fix for pkg/manifests/csv.yaml
- apiGroups:
- "config.openshift.io"
resources:
- apiservers
verbs:
- get
- list
- watch📍 Affects 3 files
pkg/manifests/csv.yaml#L72-L77(this comment)staging/operator-lifecycle-manager/deploy/chart/templates/_packageserver.clusterserviceversion.yaml#L70-L75staging/operator-lifecycle-manager/deploy/upstream/quickstart/olm.yaml#L267-L272
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/manifests/csv.yaml` around lines 72 - 77, The APIServer RBAC rules across
three files currently only grant the `get` verb, but the controller registers a
watch on configv1.APIServer via SharedInformer which requires `list` and `watch`
permissions. Update the verbs array in all three locations to include these
missing permissions. In pkg/manifests/csv.yaml at lines 72-77, add `list` and
`watch` verbs to the apiservers rule. Apply the same change to
staging/operator-lifecycle-manager/deploy/chart/templates/_packageserver.clusterserviceversion.yaml
at lines 70-75, and to
staging/operator-lifecycle-manager/deploy/upstream/quickstart/olm.yaml at lines
267-272. Each location has the same RBAC block with apiGroups containing
"config.openshift.io" and resources containing "apiservers" that needs to be
updated.
Contributor
Author
|
The upstream commit is also part of operator-framework/operator-lifecycle-manager#3849 |
tmshort
changed the title
openshift-ci-robot
added
jira/severity-important
|
@tmshort: This pull request references Jira Issue OCPBUGS-88737, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@tmshort: This pull request references Jira Issue OCPBUGS-88737, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Contributor
Author
|
/retest |
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
removed
the
needs-rebase
tmshort
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
On startup, packageserver reads the cluster-wide TLS security profile from APIServer.config.openshift.io/cluster and applies it to SecureServingOptions when --tls-min-version is not already supplied via flags. Explicit flags take precedence over the cluster profile. A 30s timeout bounds the API lookup; startup fails closed if the profile cannot be retrieved. The PSM controller now watches APIServer changes and injects --tls-min-version / --tls-cipher-suites into the packageserver CSV so that profile changes trigger a rolling restart with updated TLS settings. RBAC rules are scoped to resourceNames: [cluster] following least-privilege conventions. Unit tests cover non-OpenShift no-op, Intermediate and Modern profiles, flag precedence, and fail-closed behavior. Signed-off-by: Todd Short <tshort@redhat.com>
|
@tmshort: This pull request references Jira Issue OCPBUGS-88737, which is valid. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Contributor
Author
|
/test e2e-gcp-ovn |
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/package-server-manager/controller.go (1)
87-90: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDuplicate
--intervalflag passed to packageserver.
flagsalready contains--interval r.Interval(L88–90). Passing this slice intoreconcileCSV→ensureCSV, which itself prepends--interval intervalagain (config.goL72–74), produces--intervaltwice in the rendered run flags. Buildflagswith only the TLS additions and letensureCSVown the interval flag.🐛 Proposed fix
- flags := []string{} - if r.Interval != "" { - flags = append(flags, "--interval", r.Interval) - } + flags := []string{}Note:
requiredat L108–113 is built fromflagsbut is fully overwritten byensureCSVduringCreateOrUpdate, so dropping--intervalhere is safe;ensureCSVadds it back via theintervalargument.Also applies to: 119-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/package-server-manager/controller.go` around lines 87 - 90, The package server flag assembly is adding `--interval` twice: `controller.go` builds it into `flags`, and `ensureCSV` in `config.go` already prepends the interval flag again when rendering the CSV. Update the `reconcileCSV`/`flags` construction in `controller.go` so it only includes the TLS-related additions and omits `--interval`, letting `ensureCSV` own interval injection via its `interval` argument; also make the same adjustment anywhere else in this flow where `flags` is passed through so the rendered run flags contain a single interval value.
🧹 Nitpick comments (1)
pkg/package-server-manager/controller_test.go (1)
264-264: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering the non-nil
flagspath.The invocation correctly matches the new signature, but every case passes
nil, so the run-flag plumbing introduced by this PR (interval + caller flags) is never asserted on. A small case passing e.g.[]string{"--tls-min-version", "VersionTLS12"}and verifying it lands in the packageserver container command would guard against regressions like the duplicate---intervalissue flagged incontroller.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/package-server-manager/controller_test.go` at line 264, The current ensureCSV test coverage only exercises the nil flags path, so the new run-flag plumbing is not validated. Add a test case in controller_test.go that calls ensureCSV with non-nil flags (for example a TLS-related flag) and assert the resulting packageserver container command includes that flag along with the interval handling. Use ensureCSV and the packageserver container command construction in controller.go as the key points to verify the caller flags are propagated correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/package-server-manager/controller.go`:
- Around line 181-201: The APIServer watch in PackageServerCSVReconciler cannot
start with only named get access, so update the CSV RBAC to grant list and watch
on config.openshift.io/apiservers while keeping the existing cluster-specific
access as needed. Also change apiServerHandler to compare against
infrastructureName instead of the hardcoded "cluster" literal, and keep
SetupWithManager’s Watches on configv1.APIServer aligned with that handler.
---
Outside diff comments:
In `@pkg/package-server-manager/controller.go`:
- Around line 87-90: The package server flag assembly is adding `--interval`
twice: `controller.go` builds it into `flags`, and `ensureCSV` in `config.go`
already prepends the interval flag again when rendering the CSV. Update the
`reconcileCSV`/`flags` construction in `controller.go` so it only includes the
TLS-related additions and omits `--interval`, letting `ensureCSV` own interval
injection via its `interval` argument; also make the same adjustment anywhere
else in this flow where `flags` is passed through so the rendered run flags
contain a single interval value.
---
Nitpick comments:
In `@pkg/package-server-manager/controller_test.go`:
- Line 264: The current ensureCSV test coverage only exercises the nil flags
path, so the new run-flag plumbing is not validated. Add a test case in
controller_test.go that calls ensureCSV with non-nil flags (for example a
TLS-related flag) and assert the resulting packageserver container command
includes that flag along with the interval handling. Use ensureCSV and the
packageserver container command construction in controller.go as the key points
to verify the caller flags are propagated correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 31e9b07c-f08b-4fce-aec5-ce054975324d
📒 Files selected for processing (4)
go.modpkg/package-server-manager/config.gopkg/package-server-manager/controller.gopkg/package-server-manager/controller_test.go
Comment on lines
+181
to
+201
| func (r *PackageServerCSVReconciler) apiServerHandler(_ context.Context, obj client.Object) []reconcile.Request { | ||
| if obj.GetName() != "cluster" { | ||
| return nil | ||
| } | ||
| r.Log.Info("requeueing the packageserver deployment after encountering APIServer TLS profile change") | ||
| return []reconcile.Request{ | ||
| { | ||
| NamespacedName: types.NamespacedName{ | ||
| Name: r.Name, | ||
| Namespace: r.Namespace, | ||
| }, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| // SetupWithManager sets up the controller with the Manager. | ||
| func (r *PackageServerCSVReconciler) SetupWithManager(mgr ctrl.Manager) error { | ||
| return ctrl.NewControllerManagedBy(mgr). | ||
| For(&olmv1alpha1.ClusterServiceVersion{}). | ||
| Watches(&configv1.Infrastructure{}, handler.EnqueueRequestsFromMapFunc(r.infrastructureHandler)). | ||
| Watches(&configv1.APIServer{}, handler.EnqueueRequestsFromMapFunc(r.apiServerHandler)). |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find RBAC rules referencing apiservers and check verbs / resourceNames
rg -nP -C4 'apiservers' -g '*.yaml' -g '*.yml'
rg -nP -C4 'config\.openshift\.io' -g '*.yaml' -g '*.yml' | rg -nP -C4 'resourceNames|verbs'Repository: openshift/operator-framework-olm
Length of output: 170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== files mentioning apiservers =='
rg -n --hidden --glob '!**/vendor/**' 'apiservers|APIServer|resourceNames|cluster' .
echo
echo '== candidate RBAC / manifest files =='
fd -t f -e yaml -e yml -e json -e go . .Repository: openshift/operator-framework-olm
Length of output: 50389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== files mentioning apiservers =='
rg -n --hidden --glob '!**/vendor/**' 'apiservers|APIServer|resourceNames|cluster' .
echo
echo '== candidate RBAC / manifest files =='
fd -t f -e yaml -e yml -e json -e go . .Repository: openshift/operator-framework-olm
Length of output: 50389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focused search for controller setup, reconciler Get usage, and RBAC annotations/manifests.
rg -n --hidden --glob '!**/vendor/**' 'Watches\(&configv1\.APIServer|apiServerHandler|resourceNames: \[cluster\]|+kubebuilder:rbac|config\.openshift\.io/apiservers|client\.Get\(' pkgRepository: openshift/operator-framework-olm
Length of output: 458
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== controller file =='
sed -n '1,260p' pkg/package-server-manager/controller.go
echo
echo '== RBAC markers in package-server-manager =='
rg -n --glob 'pkg/package-server-manager/**' '\+kubebuilder:rbac|config\.openshift\.io|APIServer|apiservers|resourceNames|list|watch|get'
echo
echo '== manifest / yaml references to apiservers =='
rg -n --glob '!vendor/**' 'config\.openshift\.io/apiservers|resourceNames: \[cluster\]|resourceNames:.*cluster|verbs:|APIServer' --glob '*.yaml' --glob '*.yml' --glob '*.json'Repository: openshift/operator-framework-olm
Length of output: 7505
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== non-vendor references to APIServer RBAC / manifests =='
rg -n --hidden --glob '!vendor/**' --glob '!**/vendor/**' \
'config\.openshift\.io/apiservers|resourceNames:.*cluster|resourceNames: \[cluster\]|APIServer|apiservers|+kubebuilder:rbac' \
. --glob '*.yaml' --glob '*.yml' --glob '*.json' --glob '*.go'
echo
echo '== package-server-manager tree =='
fd -t f pkg/package-server-manager .Repository: openshift/operator-framework-olm
Length of output: 531
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== non-vendor references to APIServer RBAC / manifests =='
rg -n --hidden --glob '!vendor/**' --glob '!**/vendor/**' \
'config\.openshift\.io/apiservers|resourceNames:.*cluster|resourceNames: \[cluster\]|APIServer|apiservers|+kubebuilder:rbac' \
. --glob '*.yaml' --glob '*.yml' --glob '*.json' --glob '*.go'
echo
echo '== package-server-manager tree =='
fd -t f pkg/package-server-manager .Repository: openshift/operator-framework-olm
Length of output: 531
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== package-server-manager files =='
fd -t f pkg/package-server-manager .
echo
echo '== kubebuilder RBAC markers for config.openshift.io =='
rg -n --hidden --glob '!vendor/**' --glob '!**/vendor/**' '\+kubebuilder:rbac:groups=config\.openshift\.io' .
echo
echo '== apiservers RBAC / manifests outside vendor =='
rg -n --hidden --glob '!vendor/**' --glob '!**/vendor/**' \
'config\.openshift\.io/apiservers|resourceNames:.*cluster|resourceNames: \[cluster\]|apiservers|APIServer' \
. --glob '*.yaml' --glob '*.yml' --glob '*.json' --glob '*.go'Repository: openshift/operator-framework-olm
Length of output: 615
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== package-server-manager files =='
fd -t f pkg/package-server-manager .
echo
echo '== kubebuilder RBAC markers for config.openshift.io =='
rg -n --hidden --glob '!vendor/**' --glob '!**/vendor/**' '\+kubebuilder:rbac:groups=config\.openshift\.io' .
echo
echo '== apiservers RBAC / manifests outside vendor =='
rg -n --hidden --glob '!vendor/**' --glob '!**/vendor/**' \
'config\.openshift\.io/apiservers|resourceNames:.*cluster|resourceNames: \[cluster\]|apiservers|APIServer' \
. --glob '*.yaml' --glob '*.yml' --glob '*.json' --glob '*.go'Repository: openshift/operator-framework-olm
Length of output: 615
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== package-server-manager files =='
fd . pkg/package-server-manager
echo
echo '== kubebuilder RBAC markers for config.openshift.io =='
rg -n --hidden --glob '!vendor/**' --glob '!**/vendor/**' '\+kubebuilder:rbac:groups=config\.openshift\.io' .
echo
echo '== apiservers RBAC / manifests outside vendor =='
rg -n --hidden --glob '!vendor/**' --glob '!**/vendor/**' \
'config\.openshift\.io/apiservers|resourceNames:.*cluster|resourceNames: \[cluster\]|apiservers|APIServer' \
. --glob '*.yaml' --glob '*.yml' --glob '*.json' --glob '*.go'Repository: openshift/operator-framework-olm
Length of output: 385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== package-server-manager files =='
fd . pkg/package-server-manager
echo
echo '== kubebuilder RBAC markers for config.openshift.io =='
rg -n --hidden --glob '!vendor/**' --glob '!**/vendor/**' '\+kubebuilder:rbac:groups=config\.openshift\.io' .
echo
echo '== apiservers RBAC / manifests outside vendor =='
rg -n --hidden --glob '!vendor/**' --glob '!**/vendor/**' \
'config\.openshift\.io/apiservers|resourceNames:.*cluster|resourceNames: \[cluster\]|apiservers|APIServer' \
. --glob '*.yaml' --glob '*.yml' --glob '*.json' --glob '*.go'Repository: openshift/operator-framework-olm
Length of output: 385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== non-vendor references to config.openshift.io/APIServer =='
rg -n --hidden --glob '!vendor/**' --glob '!**/vendor/**' \
'config\.openshift\.io|APIServer|apiservers|resourceNames:.*cluster|resourceNames: \[cluster\]' \
. --glob '*.go' --glob '*.yaml' --glob '*.yml' --glob '*.json'
echo
echo '== package-server-manager controller relevant lines =='
sed -n '1,260p' pkg/package-server-manager/controller.goRepository: openshift/operator-framework-olm
Length of output: 26323
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== non-vendor references to config.openshift.io/APIServer =='
rg -n --hidden --glob '!vendor/**' --glob '!**/vendor/**' \
'config\.openshift\.io|APIServer|apiservers|resourceNames:.*cluster|resourceNames: \[cluster\]' \
. --glob '*.go' --glob '*.yaml' --glob '*.yml' --glob '*.json'
echo
echo '== package-server-manager controller relevant lines =='
sed -n '1,260p' pkg/package-server-manager/controller.goRepository: openshift/operator-framework-olm
Length of output: 26323
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '60,90p' pkg/manifests/csv.yamlRepository: openshift/operator-framework-olm
Length of output: 996
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '60,90p' pkg/manifests/csv.yamlRepository: openshift/operator-framework-olm
Length of output: 996
Grant list/watch on config.openshift.io/apiservers
pkg/manifests/csv.yamlonly grantsgetonapiserverswithresourceNames: [cluster], butWatches(&configv1.APIServer{}, ...)needs collection access. Addlist/watchso the informer can start.- Use
infrastructureNameinstead of the"cluster"literal inapiServerHandlerfor consistency.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/package-server-manager/controller.go` around lines 181 - 201, The
APIServer watch in PackageServerCSVReconciler cannot start with only named get
access, so update the CSV RBAC to grant list and watch on
config.openshift.io/apiservers while keeping the existing cluster-specific
access as needed. Also change apiServerHandler to compare against
infrastructureName instead of the hardcoded "cluster" literal, and keep
SetupWithManager’s Watches on configv1.APIServer aligned with that handler.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Apply cluster TLS security profile to packageserver serving options On startup, packageserver reads the cluster-wide TLS security profile from APIServer.config.openshift.io/cluster and applies it to SecureServingOptions
when --tls-min-version is not already supplied via flags. Explicit flags take precedence over the cluster profile. A 30s timeout bounds the API lookup; startup fails closed if the profile cannot be retrieved.
The PSM controller now watches APIServer changes and injects
--tls-min-version/--tls-cipher-suitesinto the packageserver CSV so that profile changes trigger a rolling restart with updated TLS settings.RBAC rules are scoped to resourceNames: [cluster] following least-privilege conventions. Unit tests cover non-OpenShift no-op, Intermediate and Modern profiles, flag precedence, and fail-closed behavior.
Summary by CodeRabbit